Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ncdu: 1.16 -> 2.0 #152163

Merged
merged 1 commit into from
Apr 8, 2022
Merged

ncdu: 1.16 -> 2.0 #152163

merged 1 commit into from
Apr 8, 2022

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Dec 26, 2021

Motivation for this change

Currently broken upstream it seems: https://code.blicky.net/yorhel/ncdu/issues/184

Edit: Depends on #152163 but still broken: https://code.blicky.net/yorhel/ncdu/issues/183

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@balsoft
Copy link
Member

balsoft commented Dec 26, 2021

I think this is because upstream requires Zig 0.9.0 specifically, and we have 0.8.1 in nixpkgs.

@Atemu
Copy link
Member Author

Atemu commented Dec 26, 2021

Unfortunately, that's also broken: https://code.blicky.net/yorhel/ncdu/issues/183

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@booniepepper booniepepper mentioned this pull request Jan 15, 2022
13 tasks
@booniepepper booniepepper requested a review from balsoft January 15, 2022 07:59
@Atemu
Copy link
Member Author

Atemu commented Jan 15, 2022

Unfortunately, due to #86299 preventing zig from working, this update would prevent mac users from using ncdu which is a major regression for me and many others who often use this tool.

We would have to keep the old ncdu around and make it default for mac users if we don't want to regress. This might be a good idea anyways since the ncdu 2.0 introduces new features like color some users might not want.

@toonn do you know whether the system libraries issue will be resolved any time soon?

@booniepepper
Copy link
Contributor

this update would prevent mac users from using ncdu which is a major regression for me and many others who often use this tool.

Oh ... that's no good

./c-import-order.patch # https://code.blicky.net/yorhel/ncdu/issues/183
];

XDG_CACHE_HOME="Cache"; # FIXME This should be set in stdenv
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If XDG_CACHE_HOME is unset it should fallback to $HOME/.cache. So creating $HOME should be enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an idiomatic way of doing that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be done at the program level otherwise the implementation is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We set $HOME to homeless-shelter in Nixpkgs though, don't we?

@andrewrk
Copy link
Member

andrewrk commented Jan 16, 2022

Unfortunately, due to #86299 preventing zig from working, this update would prevent mac users from using ncdu which is a major regression for me and many others who often use this tool.

This issue looks like it has been fixed in zig 0.9.0. Let me confirm this...

Edit: oops, never mind, I thought that was a zig issue for a moment 😅

Edit2: OK but I am curious why this issue would prevent Zig from working. Zig minimally depends on the system that it runs on, and thus tends to work regardless of weird system configurations. What's the issue specifically?

@Atemu
Copy link
Member Author

Atemu commented Jan 16, 2022

Next to the broken declaration in Zig's meta is a link to the issue I mentioned, so it's probably due to dependence on some macOS system library. You'd know better than me exactly what parts of a system zig depends on though ;)

@mkg20001
Copy link
Member

Would it be an idea to publish this as ncdu_2, rename the current one to ncdu_1 and then add an alias that uses v1 on mac and v2 on linux?

@bbjubjub2494
Copy link
Member

Agree with the side-by-side packaging. 2.0 won't fully replace 1.x for some time so we should keep it for a bit. According to upstream,

I will continue to maintain the C version of ncdu for as long as there are people who use it.

I think the system-dependent alias is too much magic though. Best to keep ncdu = ncdu_1 everywhere for least surprise.

@ofborg ofborg bot requested a review from SuperSandro2000 March 31, 2022 08:16
@Atemu Atemu marked this pull request as ready for review April 2, 2022 13:09
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).

Result of nixpkgs-review pr 152163 run on x86_64-linux 1

1 package built:
  • ncdu_2

@SuperSandro2000 SuperSandro2000 merged commit ece7761 into NixOS:master Apr 8, 2022
@Atemu Atemu deleted the update/ncdu branch April 8, 2022 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants